Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add property initializers #8846

Merged
merged 111 commits into from
Jun 21, 2024
Merged

feat: add property initializers #8846

merged 111 commits into from
Jun 21, 2024

Conversation

pskelin
Copy link
Contributor

@pskelin pskelin commented Apr 25, 2024

Initial values for properties

Since a web component can be used in plain HTML with just a simple tag <ui5-button></ui5-button>, all properties are optional. Providing initial (default) values for the properties used to be part of the metadata object (later also in the decorator) with a defaultValue field.

@property({ defaultValue: "abc" })
name!: string;

This actually has two mixed usages:

  • provides an initial value if none is given by the app developer
  • provides a fallback value if an invalid value is given by the app developer (mostly for numbers and enums)

With this change, component development is switching to the standard way of using property initializers

@property()
name = "abc";

Initial values

Initial values are no longer magically provided by the framework. This means that properties should be either optional or initialized, and very rarely (for complex objects) described as non-null

@property()
name1?: string; // a string that is not initialized and can be assigned undefined

@property()
name2 = "abc"; // a string (inferred type) that has an initial value and cannot be assigned undefined

@property()
wrong!: string; // this is no longer true, the framework no longer assigns "" for strings. use one of the two cases above

Fallback values

All runtime checks for properties (especially enumerations) are removed. All typechecking is left to the TypeScript compiler and assigning an invalid value to an enumeration or a number/boolean field is considered a bug that should be fixed instead of the framework silently masking it by providing a fallback value.

This means that enums are no longer supported for the type field in the decorator:

@property({ type: MyEnum }) // TS error

Migrating old code

enums

- @property({ type: PageBackgroundDesign, defaultValue: PageBackgroundDesign.Solid })
- backgroundDesign!: `${PageBackgroundDesign}`;
+ @property()
+ backgroundDesign: `${PageBackgroundDesign}` = "Solid";
  • type is no longer accepted, remove
  • default value is no longer accepted, write as initializer
  • remove ! (typescript will complain)

boolean

@property({ type: Boolean })
- noScrolling!: boolean;
+ noScrolling = false;
  • remove ! (typescript will complain)
  • initialize to false (not strictly necessary, but will infer the type boolean and is less repetition)

numbers

- @property({ validator: Integer, defaultValue: 0 })
- progress!: number;
+ @property({ type: Number })
+ progress = 0;
  • validator no longer accepted, switch to type: Number
  • default value no longer accepted, write as initializer

strings

- * @default ""
+ * @default undefined
@property()
- titleText!: string;
+ titleText?: string;
  • most strings should become optional as they are used in templates and if statements where a thruthy check is expected
  • some strings are used in ways that makes it easier to have "" (like input.value) - initialize it explicitly

DOMReference

- @property({ validator: DOMReference, defaultValue: "" })
- opener!: HTMLElement | string;
+ @property({ converter: DOMReferenceConverter })
+opener?: HTMLElement | string;
  • validator is removed, use converter

type field in decorators

The type field is now used only for attribute conversion. There are the following types

type: Boolean

treats the converstion to/from attribute as boolean

type: Number

treats the converstion to/from attribute as number

type: Array

no special handling, same as noAttribute: true

type: Object

no special handling, same as noAttribute: true

Dev time checks

Passing Number or Boolean values to attributes that are not annotated with the same type will cause a console error and fail the tests, mostly because passing such values means the type annotation was forgotten.

This means the test pages have to pass strings for fields where a string is expected (similar to how typescript will not allow passing a number to a string property)


@property({ type: Object, noAttribute: true, multiple: true })
@property({ type: Array, noAttribute: true })
_filteredItems!: Array<IComboBoxItem>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think that we have to initialise with empty array here

pskelin and others added 23 commits April 26, 2024 09:27
SAP Belize theme was deprecated as of version 1.22.0. With this change, we are performing cleanup for the deprecated theme.

BREAKING CHANGE: Remove SAP Belize theme

Related to #8461
Rename `changeWithClick` to `withScroll` in the `WizardStepChangeEventDetail`.

If the application code relied on `changeWithClic` event detail, it should now rely on `withScroll` event detail instead. The `withScroll` parameter will be set to true when the event is fired due to user scrolling.

BREAKING CHANGE: `changeWithClick` was renamed to `withScroll` in the `WizardStepChangeEventDetail`.

JIRA: BGSOFUIRILA-3867

Related to: #8461
…independently from button (#8669)

Remove the inheritance of `ToggleButton` in `SegmentedButtonItem` control. Change the `pressed` property to `selected`.

BREAKING CHANGE: The `ui5-segmentedbutton-item` `pressed` property is called `selected` now.

Previously the application developers could use the ui5-segmentedbutton-item as follows:
```html
<ui5-segmented-button>
  <ui5-segmented-button-item pressed> Option 1</ui5-segmented-button-item>
  <ui5-segmented-button-item>Option 2</ui5-segmented-button-item>
  <ui5-segmented-button-item>Option 3</ui5-segmented-button-item>
</ui5-segmented-button>
```

Now the application developers should use the ui5-segmentedbutton-item as follows:
```html
<ui5-segmented-button>
  <ui5-segmented-button-item selected> Option 1</ui5-segmented-button-item>
  <ui5-segmented-button-item>Option 2</ui5-segmented-button-item>
  <ui5-segmented-button-item>Option 3</ui5-segmented-button-item>
</ui5-segmented-button>
```

Related to: #8461
Users now can open `ui5-toast` using the `open` property. A new event `after-close` is introduced.
It may be used to sync app state with the `open` state of the `ui5-toast`.

BREAKING CHANGE: The Toast#show method has been replaced by  `open` property. If you previously used  `toast.show()` to show the toast, you must now se `toast.open=true`.

Related to: #8461
@ilhan007 ilhan007 marked this pull request as ready for review June 20, 2024 13:41
@pskelin pskelin merged commit eef0cc9 into main Jun 21, 2024
10 checks passed
@@ -210,29 +209,29 @@ class Table extends UI5Element {
/**
* Defines the accessible ARIA name of the component.
*
* @default ""
* @default undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the motivation of this change? isn't it now different than the way how DOM API works?

if an element has no id property/attribute what returns accessing the $0.id? it does not return undefined but empty string since it is defined as DOMString in the IDL. I would expect the same. Even setting undefined back $0.id = undefined should convert the undefined to 'undefined' it should not allow setting undefined back.

Same is true for enums for example for the input, setting type to e.g. $0.type="petar" returns back as text when $0.type is accessed again although the type attribute will be written as type="petar". So there is a validation for enums and silently fails to default value.

Also true for numeric values. for example for input type number, setting an invalid value $0.valueAsNumber = 'petar' will log an error and return NaN when $0.valueAsNumber is read back again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of topics to cover here, posting as separate comments

  1. undefined is a valid value for a lot of properties (there are properties that accept undefined and are initialized with undefined even before the change). accessibleName specifically is used in the AriaLabelHelper.ts and has a truthy check that treats it as undefined even when empty string is passed.
		if (accessibleEl.accessibleName) {
			return accessibleEl.accessibleName;
		}

		return undefined;

There are a lot of acc places where an empty string is different from undefined and a lot of checks for "" that return undefined (empty string is unwanted from acc perspective, but used to come from the framework).

If you still want accessible name to be initialized to "", feel free to change it. This will also remove the ? and all typescript users will be unable to assign undefined. It's up to you, but my guidance is, if undefined and "" are treated the same way, then simply go with undefined as default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. reading properties and expecting some fallback to be applied - reading properties is used in jquery style programming, real programming models like react/angular/vue will have some kind of model and will only write to the properties, or read values from events.

In this case, we didn't want to mimic existing browser behaviour if it means adding more code, especially code that introduces "magic". The typescript type already specifies valid values and components expect to work with valid values anyway.

Copy link
Contributor Author

@pskelin pskelin Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. enums and fallback - assigning an invalid value is clearly a developer error and should be fixed. If the developer assigned button.design = "Netigave" (notice the typo), it would be better to find out and correct it instead of falling back to "Default"

Enums values when used in CSS via attribute selectors already have a fallback - there should be a css rule without the attribute that will catch invalid values as well.

EDIT: enums value are also hardcoded in templates by the developers, they are not bound to some model, nor are they determined from user input. If this is the case, the application should take care to privide a default, not the framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. same for numbers - if someone passes a string to a number field, they clearly made an error that should be fixed instead of falling back to some value and masking the error. The component API clearly states that a number is expected, why should it work any other way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. in the future, ES standard decorators will provide means to differentiate what the initializer value was. Then we could use it as a fallback as well. As it is now with experimental decorators, there is no way to find out what the initializer value was - it is indistinguishable from normal setters. So what you describe could be introduced in the future.

Another option is to create a decorator (fallback, or guard) that instruments the setter and takes an addtional value, but this will be repeating the values like this which is undesired:

@property()
@fallback("text")
type = "text"

This repetition does not look good, and normal cases should be covered when using typescript with a programming model - it simply isn't necessary to have fallbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's have a call if you want a deeper discussion or use one of the regular meetings.

nnaydenow added a commit that referenced this pull request Jun 27, 2024
With the introduction of property initializers (#8846) many properties have changed they default values or types but on some places:
- JSDoc wasn't changed
- Properties with declared `declare` keyword were using wrong type
@nnaydenow nnaydenow deleted the prop-initializer branch June 27, 2024 08:59
nnaydenow added a commit that referenced this pull request Jul 1, 2024
With the introduction of property initializers (#8846) many properties have changed they default values or types but on some places:
- JSDoc wasn't changed
- Properties with declared `declare` keyword were using wrong type

This change fixes most of the things until custom element analyzer plugin is adjusted to generate correct output
nnaydenow pushed a commit that referenced this pull request Jul 2, 2024
With the introduction of property initializers (#8846) many properties have changed they default values or types but on some places:
- JSDoc wasn't changed
- Properties with declared `declare` keyword were using wrong type

This change fixes most of the things until custom element analyzer plugin is adjusted to generate correct output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants